Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Fix Install test for RN >= 0.73 #6189

Merged
merged 31 commits into from
Oct 13, 2023
Merged

Fix Install test for RN >= 0.73 #6189

merged 31 commits into from
Oct 13, 2023

Conversation

takameyer
Copy link
Contributor

@takameyer takameyer commented Oct 10, 2023 •
edited
Loading

What, How & Why?

Tests were failing for the newest React Native. The following updates have been made in this PR:

  • Separate patches for pre-0.73 on the podfile (source had changed)
  • Upgrade runners to macos13 in order to use Xcode 15, which surpases build failures for RN 0.73
    • Also use xlarge and large runners to reduce build time from 40 minutes to 6 minutes
  • Removed no longer needed patches for Hermes activation
  • Replaced simulator startup script with action (more reliable, due to simulator app location)
  • Ensure the test leaves no left over processes running and no errors (required by android emulator job)
    • Added a process.exit after the server is shutdown
    • Ensured the metro process was killed by searching for processes running on port 8081 and killing them directly
    • Ran metro with --no-interactive which removes the dev menu in the cli. This was causing an EIO error on shutdown
  • Create a small README to help in the future

☑️ ToDos

  • 📝 Changelog entry
  • 📝 Compatibility label is updated or copied from previous entry
  • 📝 Update COMPATIBILITY.md
  • 🚦 Tests
  • 📦 Updated internal package version in consuming package.jsons (if updating internal packages)
  • 📱 Check the React Native/other sample apps work if necessary
  • 💥 Breaking label has been applied or is not necessary

@takameyer takameyer changed the title First attempt Fix Install test for RN >= 0.73 Oct 10, 2023
@coveralls-official
Copy link

coveralls-official bot commented Oct 10, 2023 •
edited
Loading

Coverage Status

coverage: 85.397%. remained the same when pulling dcce394 on andrew/fix-install-tests into fb9c9e4 on main.

@takameyer takameyer force-pushed the andrew/fix-install-tests branch from ba950ea to 70161c9 Compare October 10, 2023 07:58
@takameyer takameyer force-pushed the andrew/fix-install-tests branch from b2c0d9d to aca6331 Compare October 10, 2023 09:40
@takameyer takameyer force-pushed the andrew/fix-install-tests branch from 386bcb0 to ba8c5ef Compare October 10, 2023 12:10
@takameyer takameyer force-pushed the andrew/fix-install-tests branch from ba8c5ef to 70922a9 Compare October 10, 2023 12:14
@takameyer takameyer requested a review from elle-j October 13, 2023 10:54
@takameyer takameyer marked this pull request as ready for review October 13, 2023 10:54
elle-j
elle-j approved these changes Oct 13, 2023
View reviewed changes
Copy link
Contributor

@elle-j elle-j left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for jumping deep into this 🐇 🕳️ !

.github/workflows/install-test-react-native.yml Show resolved Hide resolved
install-tests/react-native/cli.ts

// Ensure metro is really dead
// If this is removed, it's highly likely the android emulator runner will not shut down correctly
const METRO_PORT = 8081;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If Metro's port was to change for some reason, can this be loaded dynamically rather than hardcoded?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We could, but I think it's not worth the trouble until it actually happens.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've also wondered if my workaround here could be substituted by sending a stricter exit code to the .exit() method above.

@takameyer takameyer merged commit fcbae25 into main Oct 13, 2023
24 of 27 checks passed
@takameyer takameyer deleted the andrew/fix-install-tests branch October 13, 2023 13:42
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Mar 15, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@elle-j elle-j elle-j approved these changes

Assignees

@takameyer takameyer

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@takameyer @elle-j